Skip to content

fix: add missing D_and_Ds_swap() to 13 algorithms for consistent parameter labelling#147

Closed
Devguru-codes wants to merge 1 commit intoOSIPI:mainfrom
Devguru-codes:fix-missing-d-and-ds-swap
Closed

fix: add missing D_and_Ds_swap() to 13 algorithms for consistent parameter labelling#147
Devguru-codes wants to merge 1 commit intoOSIPI:mainfrom
Devguru-codes:fix-missing-d-and-ds-swap

Conversation

@Devguru-codes
Copy link
Contributor

@Devguru-codes Devguru-codes commented Feb 27, 2026

Add self.D_and_Ds_swap(results) call before return in ivim_fit() for:

  • IAR_LU_segmented_3step, IAR_LU_subtracted
  • PV_MUMC_biexp
  • OGC_AmsterdamUMC_biexp, OGC_AmsterdamUMC_biexp_segmented, OGC_AmsterdamUMC_Bayesian_biexp
  • OJ_GU_seg, PvH_KB_NKI_IVIMfit, TF_reference_IVIMfit
  • TCML_TechnionIIT_SLS, TCML_TechnionIIT_lsq_sls_lm/trf/BOBYQA

This corrects mislabelled D and D* when the optimizer swaps them. Previously only 9/22 algorithms had this correction.

No regressions: 1127 passed, 167 skipped, 22 xfailed, 6 xpassed.

Describe the changes you have made in this PR

A clear and concise description of what you want to happen

Link this PR to an issue [optional]

Fixes #146

Checklist

  • Self-review of changed code
  • Added automated tests where applicable
  • Update Docs & Guides

…meter labelling

Add self.D_and_Ds_swap(results) call before return in ivim_fit() for:
- IAR_LU_segmented_3step, IAR_LU_subtracted
- PV_MUMC_biexp
- OGC_AmsterdamUMC_biexp, OGC_AmsterdamUMC_biexp_segmented, OGC_AmsterdamUMC_Bayesian_biexp
- OJ_GU_seg, PvH_KB_NKI_IVIMfit, TF_reference_IVIMfit
- TCML_TechnionIIT_SLS, TCML_TechnionIIT_lsq_sls_lm/trf/BOBYQA

This corrects mislabelled D and D* when the optimizer swaps them.
Previously only 9/22 algorithms had this correction.

No regressions: 1127 passed, 167 skipped, 22 xfailed, 6 xpassed.
@Devguru-codes
Copy link
Contributor Author

This PR adds the missing D_and_Ds_swap() call to 13 standardized algorithm implementations that were missing it. This method corrects mislabelled D and D* (Dp) parameters when the optimizer assigns them to the wrong parameter.

Previously, only 9 out of 22 Python algorithms applied this correction, leading to inconsistent parameter labelling.

Changes Made

Added results = self.D_and_Ds_swap(results) before return results in the ivim_fit() method of these 13 files:

# File
1 IAR_LU_segmented_3step.py
2 IAR_LU_subtracted.py
3 PV_MUMC_biexp.py
4 OGC_AmsterdamUMC_biexp.py
5 OGC_AmsterdamUMC_biexp_segmented.py
6 OGC_AmsterdamUMC_Bayesian_biexp.py
7 OJ_GU_seg.py
8 PvH_KB_NKI_IVIMfit.py
9 TF_reference_IVIMfit.py
10 TCML_TechnionIIT_SLS.py
11 TCML_TechnionIIT_lsq_sls_lm.py
12 TCML_TechnionIIT_lsq_sls_trf.py
13 TCML_TechnionIIT_lsq_sls_BOBYQA.py

Each change is a single +1 line addition. No other code was modified.

Testing/Verification

  • Ran the full test suite: 1127 passed, 167 skipped, 22 xfailed, 6 xpassed — identical to main baseline
  • Zero regressions. The D_and_Ds_swap() is a no-op when parameters are already correctly labelled, so existing test results are unchanged.

@oliverchampion
Copy link
Collaborator

Some discussion and history here:
If I remember correctly, in the past, I implemented the swap in the wrapper to make sure it hit all algorithms. However, this gave some errors for edge-cases (i.e. when f=0 or f=1, we get a mono-exponential signal, for which either D or D* become meaningless. This gave some weird results (ie f=0, D fits normally, D* becomes very very small --> D and D* get swapped, messing up everything). Therefore, we chose to only implement this for some algorithms that benefited.

@IvanARashid do I remember this correctly?

@Devguru-codes
Copy link
Contributor Author

I didn't know about this. I think you are right about the edge case when f ≈ 0 (mono-exponential, no perfusion), D fits normally but D* becomes meaninglessly small (e.g. D* → 0.001). This triggers the swap condition (D > Dp AND Dp < 0.05), which incorrectly swaps D and D* and flips f, messing up an otherwise valid fit. I have 2 options here (I think and correct me if I am wrong) -
First is revert this PR or I can add a guard the swap with an additional check to avoid the edge case like this -

if results['D'] > results['Dp'] and results['Dp'] < 0.05 and results['f'] > 0.01:
    # only swap when there's a meaningful perfusion fraction

I will need your confirmation here like what to do.

@IvanARashid
Copy link
Contributor

IvanARashid commented Mar 10, 2026

Hi! So I think there has been a misunderstanding here.

It is indeed true that theoretically, D* should be greater than D. However, some of these algorithms work in specific steps that ensure that D and D* are fit using appropriate data. E.g. D* is fit using low b-values, D using high b-values. In these cases, when we get a D* < D, it is because of bad data, and not because the optimizer has swapped them. Therefore, it's a bit dangerous to use the swap function which only looks at the magnitude of these quantities and does not consider how they were fit.

Right now, the results are implemented in a way which considers how they were fit based on studying the original algorithm, which is the desired functionality.

However, this change is applicable to the algorithms that perform a bi-exponential fit in a single step, where both D and D* are fit simultaneously. If you can identify these, we could consider applying this change for those algorithms. I can for example say that the two IAR_LU algorithms are not in this category and should have the change reverted.

@oliverchampion
Copy link
Collaborator

After internal discussion, we think it makes sense to only add the swap to algorithms that requiere it and leave this up to the discretion of the coders for those algorithms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Missing D_and_Ds_swap() post-fit correction in 13 out of 22 Python algorithms

3 participants